Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tool restructuring #311

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Conversation

erikbosch
Copy link
Collaborator

@erikbosch erikbosch commented Nov 22, 2023

This is an idea I have for restructuring of vss-tools. Ass of today we have vspec2x.py as main entry point. It explicitly lists all exporters and import all of them as well. This has some drawbacks:

  • If you want to create your own tool you must customize vspec2x.py
  • The vss-tools pypi package has dependencies for all exporters. You need graphql dependencies even if you not intend to use graphql.

An alternative solution would be to instead have vspec2x as an abstract tool that require an exporter as input. That would make it easier to create you own proprietary tool/exporter, you do not need to modify vspec2x.py, instead you could just create you own exporter and give it as an argument when calling vspec2x. This PR shows how that could look like for vspec2json. The file vspec2x_abstract.py is intended to replace vspec2x.py

If we go this path we could as a second step consider refactoring our vss-tools PyPI package so that exporters with "heavy" dependencies (big size, or dependencies that only are supported on a limited set of platforms, or dependencies to less user friendly licenses) are put in a separate PyPI package. A possible example could be graphql. We could also possibly add generic exporter functionality/hooks so that exporters can add or control additional semantic checks or processing steps if needed. A typical example could be the recent discussion to expand signal tree based on allowed units. Maybe that is a feature that is possible or even mandatory for one exporter, but totally useless for others.

@SebastianSchildt
Copy link
Collaborator

Like the idea, I guess @adobekan would agree, as we have discussed a similar idea.

One main advantage would also be that you can "pip install " vss-tools, import sutff and create your custom exporter without touching COVESA upstream code. Today users are basically "forced" to fork, and I have seen such internals forks flying/rotting around. Resistance to pick up new features upstream would likely be lower, if you just need to upgrade a dependency

def main(self, arguments):
initLogging()

parser.add_argument('-I', '--include-dir', action='append', metavar='dir', type=str, default=[],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to change logic, that somehthing like vspec2json "owns" the parser for command line arguments and can include/instanctiate vpsec2x with a feature array (do I know instances/structs/uuid/extended attributes), and then vspec2x would "add" generic options , and leave options out that a tool does not support anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be quite easy to do some changes of the current solution. Today we have if , if looking for instance at vss2json.py two methods:

def feature_supported(feature_name: str)
def add_arguments(parser: argparse.ArgumentParser):

Where add_arguments is for adding exporter-specific arguments, and feature_supported to answer yes/no on features like "no expand" (for struct we have a different mechanism), but I agree that passing in some form of config array would be a nicer solution, as we then could exclude non-supported arguments already when you do -h.

@erikbosch
Copy link
Collaborator Author

Meeting notes:

  • Adnan: We have some tools that are not really exporters, but would be good if they are "close" to vss-tools. How to handle them
  • Adnan: Separate exporters from other tools
  • Erik: we could have contributed tools (not only exporters) in repo.
  • Adnan: Guidelines on how to add contributed tools

@erikbosch erikbosch force-pushed the erik_vss_restructure branch 3 times, most recently from 6db9454 to fc73044 Compare November 30, 2023 16:42
@erikbosch erikbosch force-pushed the erik_vss_restructure branch from 079ab4d to 916cb28 Compare December 21, 2023 12:27
@erikbosch erikbosch changed the title FOR DISCUSSION: Tool restructuring Tool restructuring Dec 21, 2023
@erikbosch erikbosch force-pushed the erik_vss_restructure branch 2 times, most recently from 58b17e9 to 638fd5b Compare December 21, 2023 14:44
@erikbosch erikbosch marked this pull request as ready for review December 21, 2023 15:01
@erikbosch
Copy link
Collaborator Author

Ready for review new.

@erikbosch erikbosch force-pushed the erik_vss_restructure branch 3 times, most recently from ff3b513 to 4f65f31 Compare December 22, 2023 15:12
@erikbosch erikbosch requested a review from adobekan January 22, 2024 07:52
@erikbosch
Copy link
Collaborator Author

@adobekan - could you take a look and check if you have any objections to the content of this PR. If not I will merge it.

@erikbosch
Copy link
Collaborator Author

Meeting notes:

  • Please review
  • Merge decision next meeting unless any objections

@erikbosch
Copy link
Collaborator Author

MoM: Ok to merge, after the static id PR has been merged

Refactoring tool structure so that there is not a single entrypoint (vspec2x.py)
but rather individual ones. Makes it easier to add custom generators.

Signed-off-by: Erik Jaegervall <[email protected]>
@erikbosch erikbosch force-pushed the erik_vss_restructure branch from 4f65f31 to 773ad87 Compare January 31, 2024 09:35
@erikbosch erikbosch merged commit 3db40aa into COVESA:master Jan 31, 2024
5 checks passed
@erikbosch erikbosch deleted the erik_vss_restructure branch January 31, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants